Add setting to support method implementation code lens #3876

hopehadfield · 2024-11-26T21:59:40Z

Requires eclipse-jdtls/eclipse.jdt.ls#3333

Applies to abstract and interface methods, and protected by the java.methodImplementationsCodeLens.enabled setting (false by default).

hopehadfield · 2024-11-26T22:25:49Z

@rgrunber I wasn't sure if there should be some dependency between the new setting and the java.implementationsCodeLens.enabled setting. Right now, they're independent, but I can update it if this doesn't make sense.

rgrunber · 2024-11-27T04:26:29Z

Options are either :

Rename java.implementationsCodeLens.enabled to java.typeImplementationsCodeLens.enabled and keep the other separate.
Rename java.implementationsCodeLens.enabled to something like java.implementationsCodeLens whose value is an enum : types, methods, all, none

I'm inclined towards (2). The thing with this, is it'd be ideal to add some code that still respects the old setting and it's values (true, false) if they're discovered, since users may still have them in their configuration. So true would map to types and false to none.

hopehadfield · 2024-11-27T16:26:28Z

Options are either :

Rename java.implementationsCodeLens.enabled to java.typeImplementationsCodeLens.enabled and keep the other separate.

Rename java.implementationsCodeLens.enabled to something like java.implementationsCodeLens whose value is an enum : types, methods, all, none

I'm inclined towards (2). The thing with this, is it'd be ideal to add some code that still respects the old setting and it's values (true, false) if they're discovered, since users may still have them in their configuration. So true would map to types and false to none.

Sounds good to me. I'll implement that second option with support for the old setting.

hopehadfield · 2024-11-27T18:20:13Z

@rgrunber my one concern with this approach is if users are unaware of the old setting in their configuration and intend to use the new one, then updates to the new setting won't be reflected accurately (i.e. if the old setting is true, but the user intends for the setting to be "none", then mapping the old setting to "types" and using this value for the new setting would give undesired behaviour).

Another alternative would be to keep the same name for the setting (java.implementationsCodeLens.enabled). If the user hasn't updated the setting since this change, then we map the boolean to the appropriate enum. Otherwise, it'll be overwritten by the user's update and no further action is required. Obviously not an ideal naming convention for an enum but this feels like the best solution that doesn't involve ignoring the old configuration entirely.

rgrunber · 2024-11-27T20:09:16Z

The problem with keeping it the same is that .enabled implies a boolean value in our settings. Does it help if you adopt the convention that the existence of the newer settings takes precedence over the older one ? Then you just have to detect whether the user really touched the newer setting or if it's just set to the default. I think we have code that checks that.

Have a look at isPreferenceOverridden and see if that does the trick.

hopehadfield · 2024-11-29T02:53:42Z

Yes that would definitely work, I didn't know if we could check that the default had been intentionally set or not.

I can't find isPreferenceOverridden in jdt.ls, jdt.ui, or jdt.core... not sure if I'm overlooking it or if it's in a different package?

rgrunber · 2024-11-29T04:33:44Z

vscode-java/src/utils.ts

Line 14 in 309c705

export function isPreferenceOverridden(section: string): boolean {

hopehadfield · 2024-11-29T18:25:44Z

Ah, I was looking to do the check on the server-side.

I tried using isPreferenceOverridden but it evaluates as false when the setting is set to the default value (even if it has been set to another value and then set back to default)... not sure what the way around this is. It feels like we would want the new setting value to be respected so long as the user has accessed/viewed the new setting, though I'm not sure this is possible?

rgrunber · 2024-11-29T21:44:05Z

Ah, I was looking to do the check on the server-side.

I forgot that we might have to do the same on the server side for other clients 😐 . I guess we can look at that later. I feel like we made this decision once before on another PR.

I tried using isPreferenceOverridden but it evaluates as false when the setting is set to the default value (even if it has been set to another value and then set back to default)... not sure what the way around this is. It feels like we would want the new setting value to be respected so long as the user has accessed/viewed the new setting, though I'm not sure this is possible?

This seems more like a quirk of VS Code. When a value is set to match the default through the Settings UI, VS Code removes the setting from the underlying settings.json file as if it were just following the default. Seems like that's a recipe for disaster if the default ever changes, but I'm fine with treating that as "not overridden" given that it's VS Codes fault.

However, I don't think we have to worry about that because when the updated extension is installed, there will be no "boolean" checkbox for java.implementationsCodeLens.enabled. The old setting won't be "visible", and the new (java.implementationsCodeLens) one will have a dropdown. They'll only be able to set the old value through the settings.json file and isPreferenceOverridden will detect that.

Let me know if that makes sense.

hopehadfield · 2024-12-10T23:22:31Z

Sorry for the delay, got swamped with studying for exams. I updated the setting, though please note that I named it java.implementationCodeLens as opposed to java.implementationsCodeLens because isPreferenceOverridden would evaluate to true for the updated setting (even if it wasn't set) so long as java.implementationsCodeLens.enabled was defined, making the check redundant.

Please let me know if this update works.

rgrunber

Overall, look good and mostly ready to merge. Just a minor logic issue.

I've played with this and it seems to work well in supporting the deprecated option.

rgrunber · 2024-12-13T17:20:54Z

src/utils.ts

@@ -272,6 +272,14 @@ export async function getJavaConfig(javaHome: string) {
 		const userConfiguredJREs: any[] = javaConfig.configuration.runtimes;
 		javaConfig.configuration.runtimes = await addAutoDetectedJdks(userConfiguredJREs);
 	}
+
+	if (!isPreferenceOverridden("java.implementationCodeLens") && javaConfig.implementationsCodeLens){


This is probably good enough. Did you see this approach somewhere in the code ? What do you think of :

const deprecatedImplementations = typeof javaConfig.implementationsCodeLens?.enabled === 'boolean';

Then you won't need that undefined check or the risk that javaConfig.implementationsCodeLens.enabled might be something other than a boolean that gets converted to a boolean.

Isn't this expression just evaluating if javaConfig.implementationsCodeLens.enabled is a boolean or not? If enabled is false, this would return true.

What about

if (!isPreferenceOverridden("java.implementationCodeLens") && typeof javaConfig.implementationsCodeLens?.enabled === 'boolean'){ const deprecatedImplementations = javaConfig.implementationsCodeLens.enabled; javaConfig.implementationCodeLens = deprecatedImplementations ? "types" : "none";

👍 Yeah I think we agree. I just wasn't specific. You'd just have 2 different values. One for whether the property is a boolean, and one for the actual boolean value. Here's what I had :

diff --git a/src/utils.ts b/src/utils.ts index 0a06f45..58f8c22 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -273,11 +273,9 @@ export async function getJavaConfig(javaHome: string) { javaConfig.configuration.runtimes = await addAutoDetectedJdks(userConfiguredJREs); } - if (!isPreferenceOverridden("java.implementationCodeLens") && javaConfig.implementationsCodeLens){ - const deprecatedImplementations: boolean | undefined = javaConfig.implementationsCodeLens.enabled; - if (deprecatedImplementations !== undefined){ - javaConfig.implementationCodeLens = deprecatedImplementations ? "types" : "none"; - } + const deprecatedImplementations = typeof javaConfig.implementationsCodeLens.enabled === 'boolean'; + if (!isPreferenceOverridden("java.implementationCodeLens") && deprecatedImplementations){ + javaConfig.implementationCodeLens = javaConfig.implementationsCodeLens.enabled ? "types" : "none"; } return javaConfig;

I think either is fine.

Signed-off-by: Hope Hadfield <[email protected]>

hopehadfield · 2024-12-17T17:13:42Z

Thanks for the advice and review! 😄 @rgrunber

hopehadfield mentioned this pull request Nov 26, 2024

Add support for interface and abstract method implementation code lens eclipse-jdtls/eclipse.jdt.ls#3333

Merged

hopehadfield requested a review from rgrunber November 26, 2024 22:25

rgrunber reviewed Dec 13, 2024

View reviewed changes

Add setting to support method implementation code lens

a450004

Signed-off-by: Hope Hadfield <[email protected]>

hopehadfield force-pushed the 3813-method-implementations branch from d7aab29 to a450004 Compare December 16, 2024 21:56

rgrunber merged commit 3297423 into redhat-developer:master Dec 17, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting to support method implementation code lens #3876

Add setting to support method implementation code lens #3876

hopehadfield commented Nov 26, 2024

hopehadfield commented Nov 26, 2024

rgrunber commented Nov 27, 2024 •

edited

Loading

hopehadfield commented Nov 27, 2024

hopehadfield commented Nov 27, 2024

rgrunber commented Nov 27, 2024 •

edited

Loading

hopehadfield commented Nov 29, 2024

rgrunber commented Nov 29, 2024

hopehadfield commented Nov 29, 2024

rgrunber commented Nov 29, 2024

hopehadfield commented Dec 10, 2024

rgrunber left a comment •

edited

Loading

rgrunber Dec 13, 2024

hopehadfield Dec 16, 2024

rgrunber Dec 16, 2024 •

edited

Loading

hopehadfield commented Dec 17, 2024

Add setting to support method implementation code lens #3876

Add setting to support method implementation code lens #3876

Conversation

hopehadfield commented Nov 26, 2024

hopehadfield commented Nov 26, 2024

rgrunber commented Nov 27, 2024 • edited Loading

hopehadfield commented Nov 27, 2024

hopehadfield commented Nov 27, 2024

rgrunber commented Nov 27, 2024 • edited Loading

hopehadfield commented Nov 29, 2024

rgrunber commented Nov 29, 2024

hopehadfield commented Nov 29, 2024

rgrunber commented Nov 29, 2024

hopehadfield commented Dec 10, 2024

rgrunber left a comment • edited Loading

Choose a reason for hiding this comment

rgrunber Dec 13, 2024

Choose a reason for hiding this comment

hopehadfield Dec 16, 2024

Choose a reason for hiding this comment

rgrunber Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

hopehadfield commented Dec 17, 2024

rgrunber commented Nov 27, 2024 •

edited

Loading

rgrunber commented Nov 27, 2024 •

edited

Loading

rgrunber left a comment •

edited

Loading

rgrunber Dec 16, 2024 •

edited

Loading